-
Notifications
You must be signed in to change notification settings - Fork 97
Fix Unwanted space insertion in single-line MarkdownTextInput when replacing selected text #717
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…placing selected text
|
All contributors have signed the CLA ✍️ ✅ |
|
Fix Summary What changes do you think we should make in order to solve the problem?We should make newline addition conditional on the
Current problematic code: if (index < lines.length - 1 || (index === 0 && line === '')) {
addBrElement(node);
}Why change is needed: This code always adds BR elements (containing \n characters) regardless of whether the input should be single-line or multiline. For Required change: // Only add BR elements for multiline inputs or when there are actual line breaks
if (isMultiline && (index < lines.length - 1 || (index === 0 && line === ''))) {
addBrElement(node);
}
Current problematic code: // Line 232
addTextToElement(currentParentNode, line.text);
// Line 258
addTextToElement(currentParentNode, textBeforeRange);
// Line 288
addTextToElement(currentParentNode, textAfterRange);Why change is needed: These function calls don't pass the isMultiline parameter, causing them to default to true and always add BR elements even for single-line Required changes: // Line 232
addTextToElement(currentParentNode, line.text, isMultiline);
// Line 258
addTextToElement(currentParentNode, textBeforeRange, isMultiline);
// Line 288
addTextToElement(currentParentNode, textAfterRange, isMultiline);
Current function signature: function parseInnerHTMLToText(target: MarkdownTextInputElement, cursorPosition: number, inputType?: string): stringWhy change is needed: The function doesn't know whether the input should be single-line or multiline when extracting text from DOM, so it always assumes multiline Required change: function parseInnerHTMLToText(target: MarkdownTextInputElement, cursorPosition: number, inputType?: string, isMultiline = true): string
Current problematic code: if (shouldAddNewline && !containsEmptyBlockElement) {
text += '\n';
shouldAddNewline = false;
}
shouldAddNewline = true;Why change is needed: This code always adds newlines between top-level components, even for single-line inputs where newlines should not be added. Required change: // Only add newlines for multiline inputs
if (isMultiline && shouldAddNewline && !containsEmptyBlockElement) {
text += '\n';
shouldAddNewline = false;
}
shouldAddNewline = isMultiline;
Current problematic code: if (parentNode && parentNode.parentElement?.contentEditable !== 'true' && !!node.getAttribute('data-id')) {
// Parse br elements into newlines only if their parent is not a child of the MarkdownTextInputElement
text += '\n';
}Why change is needed: This code converts BR elements to newlines regardless of whether the input should be single-line, causing unwanted newlines in the extracted Required change: if (isMultiline && parentNode && parentNode.parentElement?.contentEditable !== 'true' && !!node.getAttribute('data-id')) {
// Parse br elements into newlines only if their parent is not a child of the MarkdownTextInputElement - and now only for multiline inputs
text += '\n';
}
Current problematic code: let parsedText = normalizeValue(
inputType === 'pasteText' ? pasteContent.current || '' : parseInnerHTMLToText(e.target as MarkdownTextInputElement, contentSelection.current.start, inputType),
);Why change is needed: The function call doesn't pass the multiline prop, so parseInnerHTMLToText() defaults to multiline behavior even for single-line inputs. Required change: let parsedText = normalizeValue(
inputType === 'pasteText' ? pasteContent.current || '' : parseInnerHTMLToText(e.target as MarkdownTextInputElement, contentSelection.current.start, inputType,
multiline),
); |
|
cc @jmusial Will you take a look? |
jmusial
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to me that minimal fix requires only 3, 4 and 6.
2 seems reasonable addition. Would you mind adding test cases for that as well ?
Also please add before & after videos showcasing the changes and test the solution with E/App
src/web/utils/inputUtils.ts
Outdated
| const containsEmptyBlockElement = firstChild?.getAttribute?.('data-type') === 'block' && firstChild.textContent === ''; | ||
| if (shouldAddNewline && !containsEmptyBlockElement) { | ||
| // Only add newlines for multiline inputs | ||
| if (isMultiline && shouldAddNewline && !containsEmptyBlockElement) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we rather
if (!isMultiline && !inputType && node.nodeType === Node.TEXT_NODE) {
in line #70 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we discuss this change here, rather than adding in the comments ? It helps keeping the context :)
Your'e correct , there is a clerical error in my proposed solution, should be
if (!isMultiline || (!inputType && node.nodeType === Node.TEXT_NODE)) {
should handle all the cases.
Hello @jmusial Before: Since the before is similar to Expensify/App#64831 issue screenshot, I just referenced it. |
Thank you for the feedback! You're absolutely right that changes 3, 4, and 6 form the core of the fix. However, I'd like to respectfully suggest that change 1 is also critical for a complete solution. Why Change 1 (BR element fix) is necessary:The issue occurs in two places where newlines are introduced:
Without change 1, single-line inputs will still have BR elements inserted into the DOM, which can cause issues in other scenarios beyond just the text extraction phase. |
|
Also Here comprehensive test cases covering:
|
yup, test scenarios look good, please implement |
387eea1 to
3bc2e59
Compare
jmusial
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@tomekzaw do you want to take a look as well ?
|
@TaduJR I think you need to still sign the CLA |
|
I have read the CLA Document and I hereby sign the CLA |
|
Just signed it @jmusial |
tomekzaw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the PR!
allroundexperts
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
|
@TaduJR I can't merge the PR because of the following error:
Please sign your commits retroactively (or submit another PR if that's easier). |
3bc2e59 to
1f16547
Compare
1f16547 to
722627e
Compare


Fixes #716
Details
This PR fixes a bug where
MarkdownTextInputwithmultiline={false}incorrectly inserts spaces when users select all text and type a replacement character. The issue was caused by the library treating all text input as potentially multiline during processing, regardless of themultilineprop value.Root Cause:
The library was adding BR elements (containing
\ncharacters) and newlines during both DOM generation and text extraction phases, even for single-line inputs. These newlines then got converted to spaces during text normalization.Changes Made:
1. Fixed BR element addition in
addTextToElement()function (parserUtils.ts)isMultilineparameter2. Updated
addTextToElement()function calls (parserUtils.ts)isMultilineparameter to all function calls withinparseRangesToHTMLNodes()3. Enhanced
parseInnerHTMLToText()function (inputUtils.ts)isMultiline = trueparameter to maintain backward compatibility4. Connected multiline prop in
MarkdownTextInput.web.tsxmultilineprop toparseInnerHTMLToText()functionImpact:
Related Issues
GH_LINK
Manual Tests
Test Scenarios Covered:
Single-line text replacement:
MarkdownTextInputwithmultiline={false}Multiline preservation:
MarkdownTextInputwithmultiline={true}Markdown highlighting:
Edge cases:
Regression testing:
Manual Testing Steps:
multiline={false}andtype="markdown"